Skip to content

Extract logical plans in LogicalPlan as independent struct: TableScan#1290

Merged
alamb merged 1 commit into
apache:masterfrom
xudong963:extract_logical_plan
Nov 16, 2021
Merged

Extract logical plans in LogicalPlan as independent struct: TableScan#1290
alamb merged 1 commit into
apache:masterfrom
xudong963:extract_logical_plan

Conversation

@xudong963

@xudong963 xudong963 commented Nov 12, 2021 •
edited by alamb
Loading

Copy link
Copy Markdown
Member

Which issue does this PR close?

Related to #1228

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@xudong963

xudong963 commented Nov 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I extracted TableScan as an example, PTAL @alamb @houqp

I have two questions:

  1. How about extracting every plan to a single file.
  2. Extract All plans at once as one ticket or divide them into multiple tickets? I think dividing into multiple tickets will be good to review. (maybe every ticket is less than 200 lines.)

@liukun4515

liukun4515 commented Nov 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

@xudong963, If you split them into multiple tasks, you can list each subtask, may be I can help you to do someone.

alamb
alamb approved these changes Nov 13, 2021
View reviewed changes

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @xudong963 - this PR looks exactly like what is discussed on #1228 👍

I think this looks like an improvement to me. It will be a fairly large, but mechanical change to the codebase but I do think it will help over time

What do you think about this change @jimexist @houqp @rdettai and @Dandandan

@xudong963

xudong963 commented Nov 13, 2021

Copy link
Copy Markdown
Member Author

Thank you for the contribution @xudong963 - this PR looks exactly like what is discussed on #1228 👍

I think this looks like an improvement to me. It will be a fairly large, but mechanical change to the codebase but I do think it will help over time

My pleasure! I believe after finishing this, it'll bring help for subqueries implementation.

@houqp houqp added enhancement New feature or request and removed enhancement New feature or request labels Nov 15, 2021
This was referenced Nov 15, 2021
Closed
Closed
Closed
Closed
Closed
Closed
@xudong963

xudong963 commented Nov 15, 2021

Copy link
Copy Markdown
Member Author

The ticket can be merged? Then we can push related tickets🎯

@alamb

alamb commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

The ticket can be merged? Then we can push related tickets🎯

Any concerns @Dandandan / @andygrove / @jimexist / @rdettai ?

rdettai
rdettai reviewed Nov 15, 2021
View reviewed changes

@rdettai rdettai left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work @xudong963. I am not sure I fully understand the big picture of the change though. It seems from #1228 that this is part of adding subqueries, but I would find it clearer if there was an umbrella issue with bullet points that list to steps/issues.

I have also raised some questions here: #1228 (comment)

@alamb

alamb commented Nov 16, 2021

Copy link
Copy Markdown
Contributor

There is some good discussion on #1228 (comment) -- to be clear, in my mind, the purpose of this refactoring is to make LogicalPlan nodes easier to work with longer term.

Thanks @xudong963 for leading the charge

@alamb alamb merged commit 79f129d into apache:master Nov 16, 2021
@xudong963 xudong963 deleted the extract_logical_plan branch November 16, 2021 11:44
matthewmturner pushed a commit to matthewmturner/arrow-datafusion that referenced this pull request Nov 16, 2021
@xudong963 @matthewmturner
Extract logical plans in LogicalPlan as independent struct: TableScan (…
d0cd143 
…apache#1290)
This was referenced Nov 17, 2021
Merged
Merged
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@erenavsarogullari
Feat: Support array_join function (apache#1290)
9a4e5b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes
@houqp houqp houqp approved these changes
+1 more reviewer
@rdettai rdettai rdettai left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xudong963 @liukun4515 @alamb @houqp @rdettai